Skip to content

Feat/model inputs dump#1118

Open
JackTan25 wants to merge 1 commit into
mainfrom
feat/model_inputs_dump
Open

Feat/model inputs dump#1118
JackTan25 wants to merge 1 commit into
mainfrom
feat/model_inputs_dump

Conversation

@JackTan25

Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings June 18, 2026 08:23
@JackTan25 JackTan25 requested a review from LLLLKKKK as a code owner June 18, 2026 08:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds an opt-in debug feature to dump model inputs into rotating log files to aid troubleshooting and profiling.

Changes:

  • Introduces enable_model_inputs_log config/CLI/env flag and propagates it through executors and gatherers.
  • Extends GptModelInputs with extra tensors for logging snapshots and wires a new ModelInputsLogger into PyWrappedModel::forward.
  • Implements ModelInputsLogger with file rotation, periodic flush, and optional metrics reporting.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
rtp_llm/server/server_args/profile_debug_logging_group_args.py Adds CLI/env switch to enable model input logging.
rtp_llm/cpp/config/ConfigModules.h Adds enable_model_inputs_log to profiling debug config.
rtp_llm/cpp/config/ConfigModules.cc Prints the new config field in to_string().
rtp_llm/cpp/pybind/ConfigInit.cc Exposes the new config field to Python + extends pickle state.
rtp_llm/models_py/bindings/core/OpData.h Adds host snapshot tensors to GptModelInputs for logging.
rtp_llm/cpp/normal_engine/NormalModelInputGatherer.h Adds gatherer flag to control host snapshots for logging.
rtp_llm/cpp/normal_engine/NormalModelInputGatherer.cc Populates logging snapshot tensors when enabled.
rtp_llm/cpp/normal_engine/NormalBatchStreamProcessor.cc Passes the new flag into the input gatherer config.
rtp_llm/cpp/normal_engine/NormalExecutor.h Stores a shared ModelInputsLogger in the executor.
rtp_llm/cpp/normal_engine/NormalExecutor.cc Conditionally constructs/injects ModelInputsLogger into model.
rtp_llm/cpp/normal_engine/speculative/MtpExecutor.h Stores a shared ModelInputsLogger for speculative executor.
rtp_llm/cpp/normal_engine/speculative/MtpExecutor.cc Conditionally constructs/injects ModelInputsLogger into models.
rtp_llm/cpp/models/PyWrappedModel.h Adds logger dependency to constructor and stores it.
rtp_llm/cpp/models/PyWrappedModel.cc Emits model inputs logs at start of forward().
rtp_llm/cpp/models/ModelInputsLogger.h Adds new logger class interface.
rtp_llm/cpp/models/ModelInputsLogger.cc Implements JSONL logging + rotation + metrics.
rtp_llm/cpp/models/BUILD Adds build deps needed by ModelInputsLogger.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread rtp_llm/cpp/models/ModelInputsLogger.cc Outdated
Comment on lines +229 to +240
output_.open(file_path_, std::ios::out | std::ios::trunc);
bytes_ = 0;
}

std::mutex mutex_;
std::filesystem::path file_path_;
std::ofstream output_;
int backup_count_ = 0;
size_t bytes_ = 0;
size_t pending_lines_ = 0;
int64_t last_flush_us_ = 0;
bool valid_ = false;
Comment thread rtp_llm/cpp/models/ModelInputsLogger.cc Outdated
Comment on lines +31 to +56
std::string jsonEscape(const std::string& input) {
std::ostringstream os;
for (unsigned char c : input) {
switch (c) {
case '\\':
os << "\\\\";
break;
case '"':
os << "\\\"";
break;
case '\n':
os << "\\n";
break;
case '\r':
os << "\\r";
break;
case '\t':
os << "\\t";
break;
default:
os << static_cast<char>(c);
break;
}
}
return os.str();
}
Comment thread rtp_llm/cpp/models/ModelInputsLogger.cc Outdated
Comment on lines +58 to +65
std::string combineStringsForLog(const std::vector<std::string>& vec) {
std::string result = "\" ";
for (const auto& s : vec) {
result += s + ", ";
}
result += "\"";
return result;
}
Comment on lines +99 to +106
profile_debug_logging_group.add_argument(
"--enable_model_inputs_log",
env_name="ENABLE_MODEL_INPUTS_LOG",
bind_to=(profiling_debug_config, "enable_model_inputs_log"),
type=str2bool,
default=False,
help="控制是否打印模型输入日志。可选值: True (启用), False (禁用)。默认为 False",
)
Comment on lines +42 to +45
torch::Tensor combo_tokens_host_for_log;
torch::Tensor input_lengths_host_for_log;
torch::Tensor sequence_lengths_host_for_log;
torch::Tensor prefix_lengths_host_for_log;
@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from 6eab357 to c8deafc Compare June 18, 2026 08:39
@LLLLKKKK

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1118

Status: LGTM

Summary: P0/0 · P1/0 · P2/3 · P3/0

lgtm ready to ci

Non-blocking Suggestions

P2

  • MTP 路径会记录过期的模型输入快照 @ rtp_llm/cpp/models/ModelInputsLogger.cc:132
    • 建议:在每次修改 GptModelInputs 后同步更新或清空 *_host_for_log,或仅在快照与当前 tensor 同源时使用快照。
  • 新增模型输入日志链路缺少测试覆盖 @ rtp_llm/cpp/models/ModelInputsLogger.cc:248
    • 建议:补充 logger 单测或 executor/gatherer 测试,覆盖普通 decode/prefill、MTP 修改输入后的日志内容和默认关闭路径。
  • decode 日志的顶层 id 恒为 -1 @ rtp_llm/cpp/models/ModelInputsLogger.cc:100
    • 建议:为 decode batch 填充 request_id,或在 request_id 为空时退化使用 trace_ids/stream id 生成可关联的日志 id。

Checklist Violations (6 fail / 78 total)

General Principles Checklist

  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue MTP 路径会记录过期的模型输入快照
    _*host_for_log 快照在 gather 后设置,但 MTP 后续会改写 GptModelInputs 再 forward,日志状态会与实际输入不一致。
  • [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue decode 日志的顶层 id 恒为 -1
    ModelInputsLogger.cc 的 firstRequestId 只读 request_id,decode-only batch 为空时顶层 id 为 -1,削弱请求关联。
  • [6.1] Tests — 新逻辑有聚焦单测 + 相关集成/smoke 测试 → issue 新增模型输入日志链路缺少测试覆盖
    diff_paths 没有测试文件,新增日志格式、开关传播和 MTP 快照一致性未被自动覆盖。
  • [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue 新增模型输入日志链路缺少测试覆盖
    日志轮转、空 tensor、CUDA tensor、长输入截断和 MTP 多次 forward 等边界未见测试覆盖。
  • [6.1] Tests — 分布式/跨平台变更有对应覆盖 → checklist-only
    新增日志会在 TP rank 上执行,但功能默认关闭;建议后续覆盖多 rank dump 一致性,暂不单独升级为 issue。

RTP-LLM Checklist

  • [H] 测试与 CI — 测试覆盖充分:大重构等价覆盖,新功能端到端测试 → issue 新增模型输入日志链路缺少测试覆盖
    新增模型输入日志功能无对应测试文件。

Strengths

  • 新增开关默认关闭,且 ProfilingDebugLoggingConfig 的 pybind pickle 对旧 12 元组保留兼容。
  • 日志 writer 有大小轮转、批量 flush 和耗时 metric,便于控制排查功能的运行成本。

Comment thread rtp_llm/cpp/models/ModelInputsLogger.cc Outdated
return pos == std::string::npos ? tensor_with_data : tensor_with_data.substr(pos + 2);
}

std::string tensorLogString(const torch::Tensor& tensor, const torch::Tensor& host_snapshot = {}) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

搞这些干啥?直接 torch dump 就行了

@@ -0,0 +1,271 @@
#include "rtp_llm/cpp/models/ModelInputsLogger.h"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replay 也要做了

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

正常调用都是在模型执行之前打印的日志,relay也可以打印出来

Copilot AI review requested due to automatic review settings June 26, 2026 02:51
@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from c8deafc to aacc636 Compare June 26, 2026 02:51
@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from aacc636 to c8deafc Compare June 26, 2026 02:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings June 26, 2026 02:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Comment thread rtp_llm/cpp/models/ModelInputsLogger.cc Outdated
Comment on lines +178 to +181
const auto server_id = autil::EnvUtil::getEnv("FRONTEND_SERVER_ID", 0);
file_path_ = log_dir / ("model_inputs_r" + std::to_string(rank_id) + "_s" + std::to_string(server_id) + ".log");
bytes_ = std::filesystem::exists(file_path_, ec) ? std::filesystem::file_size(file_path_, ec) : 0;
output_.open(file_path_, std::ios::out | std::ios::app);
Comment thread rtp_llm/cpp/models/ModelInputsLogger.cc Outdated
Comment on lines +193 to +196
std::lock_guard<std::mutex> guard(mutex_);
rotateIfNeeded(line.size() + 1);
output_ << line << '\n';
bytes_ += line.size() + 1;
@LLLLKKKK

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1118

Status: BLOCKING

Summary: P0/0 · P1/1 · P2/3 · P3/2

Blocking Issues

P1

  • per-forward GPU 同步:启用 model_inputs_log 后 tensorForDump 对 CUDA tensor 调用 .cpu() 造成隐式同步 @ rtp_llm/cpp/models/ModelInputsLogger.cc:34
    • 建议:将 log() 中的序列化和磁盘写入移到后台线程(生产者-消费者队列),仅在 forward 路径做快速的 tensor snapshot(已有 host_for_log 的思路扩展到所有 tensor),避免阻塞推理热路径。

Non-blocking Suggestions

P2

  • backup_count 参数被接收但未使用,dump 文件无清理机制,存在磁盘空间泄漏风险 @ rtp_llm/cpp/models/ModelInputsLogger.cc:98
    • 建议:实现基于 backup_count 的文件数量限制(删除最旧文件),或移除该参数并在文档中说明需外部清理。
  • ofstream 写入后未检查错误状态,磁盘满时静默产生空/损坏文件 @ rtp_llm/cpp/models/ModelInputsLogger.cc:148
    • 建议:close() 后检查 if (!output) { LOG_WARNING(...); std::filesystem::remove(tmp_path, ec); return; },失败时清理 tmp 文件跳过 rename。
  • _init_dsv4_kv_cache 中 kernel_tokens 为 0 时会触发 ZeroDivisionError @ rtp_llm/models_py/standalone/auto_model.py:212
    • 建议:在 _init_dsv4_kv_cache 入口处校验 kernel_tokens > 0,否则 raise ValueError 提示必须设置 kernel_tokens_per_block。

P3

  • _InitResources 使用裸 class + monkey-patch 赋值属性 @ rtp_llm/models_py/standalone/auto_model.py:314
    • 建议:使用 dataclass 或 SimpleNamespace 替代裸 class,明确声明字段。
  • hasattr 做控制流分支 @ rtp_llm/models_py/standalone/auto_model.py:322
    • 建议:使用 Protocol + isinstance 或 try/except AttributeError 替代 hasattr 控制流。

Checklist Violations (7 fail / 78 total)

General Principles Checklist

  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue backup_count 参数被接收但未使用,dump 文件无清理机制,存在磁盘空间泄漏风险
    dump 文件无限累积无清理路径。backup_count 参数被声明但实现中忽略,API 契约暗示有轮转保护但实际没有。output_dir_valid 和 init_once_ 状态管理正确。_
  • [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue ofstream 写入后未检查错误状态,磁盘满时静默产生空/损坏文件
    ofstream 写入失败时流状态未检查,损坏 tmp 文件被 rename 为正式 .pt。目录创建和文件打开失败有正确处理。

RTP-LLM Checklist

  • [D] 性能 — 设备同步 .item()/.cpu()/.tolist() 在关键路径 → issue per-forward GPU 同步:启用 model_inputs_log 后 tensorForDump 对 CUDA tensor 调用 .cpu() 造成隐式同步
    tensorForDump() 对无 host_for_log 快照的 CUDA tensor(如 kv_cache_block_id)调用 .cpu() 触发隐式 GPU sync,在 forward 热路径上。

Python Static-First Checklist

  • [P.A] 静态结构与类型纪律 — 禁止 getattr/setattr literal 访问 → checklist-only
    _auto_model.py init_dsv4_kv_cache 中 getattr(self.model_config.attn_config, 'layer_compress_ratios', None) 和 getattr(self.model_config, 'gen_num_per_cycle', 0) 用字面量 key。HuggingFace config 属性动态加载,getattr 有合理性但不符合静态优先原则。因 standalone 工具且属性确实可能不存在,不升级为 issue。
  • [P.A] 静态结构与类型纪律 — 禁止 hasattr 做控制流分支 → issue hasattr 做控制流分支
    auto_model.py:322 if hasattr(self.model, 'initialize') 和 replay_test.py hasattr(auto_model.model, 'prepare_fmha_impl') 做控制流分支。
  • [P.A] 静态结构与类型纪律 — 数据容器用 dataclass/NamedTuple/TypedDict → issue _InitResources 使用裸 class + monkey-patch 赋值属性
    __initialize_py_model 中 class InitResources: pass 后通过属性赋值填充字段,缺少静态类型声明。
  • [P.F] 语言陷阱 — 禁止可变默认参数 → checklist-only
    AutoModel.init 参数 stop_id_list: list[int] = [] 使用可变默认值。代码使用前做了 .copy() 防御,但可变默认对象在所有调用间共享仍是隐患。本 PR 正在修改该函数签名,适合一并修复为 None 默认值。

Strengths

  • host_for_log 快照机制在 gatherer 层预先捕获 CPU tensor 副本,避免 dump 时大部分 CUDA tensor 的 D2H 同步开销
  • 原子写入模式(先写 .tmp 再 rename)保证 dump 文件完整性,避免读到半写文件
  • pickle 反序列化兼容旧版本(tuple size 12 和 13),遵循项目已有的兼容性模式
  • C++ dump tool + Python replay test 形成跨语言端到端测试闭环,验证序列化格式一致性

Copilot AI review requested due to automatic review settings June 29, 2026 11:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@LLLLKKKK

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1118

Status: BLOCKING

Summary: P0/0 · P1/1 · P2/0 · P3/1

Blocking Issues

P1

  • backup_count 参数被忽略,dump 文件无限增长无清理机制 @ rtp_llm/cpp/models/ModelInputsLogger.cc:97
    • 建议:实现基于 backup_count 的文件轮转机制:维护一个文件计数器,当文件数超过 backup_count 时删除最旧的 dump 文件;或者使用时间窗口定期清理。若暂不实现,删除 backup_count 参数避免误导调用方。

Non-blocking Suggestions

P3

  • ofstream 写入完成后未检查流状态 @ rtp_llm/cpp/models/ModelInputsLogger.cc:149
    • 建议:在 close() 后检查 output.good();失败时删除临时文件并打印 WARNING。

Checklist Violations (5 fail / 74 total)

RTP-LLM Checklist

  • [D] 性能 — 硬编码容量值 → issue backup_count 参数被忽略,dump 文件无限增长无清理机制
    backup_count 参数从 profiling_debug_logging_config.log_file_backup_count(默认 16)传入但被注释掉未使用(int /*backup_count*/),dump 文件无限增长无轮转清理机制。

Python Static-First Checklist

  • [P.A] 静态结构与类型纪律 — 禁止 getattr/setattr literal 访问 → checklist-only
    auto_model.py:177 getattr(self.model_config.attn_config, "layer_compress_ratios", None) 和 :215 getattr(self.model_config, "gen_num_per_cycle", 0)。因 pybind11 config 对象跨版本可能缺少属性,在 standalone 测试工具中此模式可接受,不升级为独立 issue。
  • [P.A] 静态结构与类型纪律 — 禁止 hasattr 做控制流分支 → checklist-only
    auto_model.py:322 hasattr(self.model, "initialize") 用于决定初始化路径。模型通过 ModelFactory 动态加载,接口不统一,hasattr duck-typing 在 standalone 工具中可接受,不升级为独立 issue。
  • [P.A] 静态结构与类型纪律 — 数据容器用 dataclass/NamedTuple/TypedDict → checklist-only
    _auto_model.py:314 class _InitResources: pass 是空类,字段通过外部动态赋值设置。但该类仅在 initialize_py_model 内部局部使用,生命周期极短且 scope 极小,在 standalone 诊断工具中可接受,不升级为独立 issue。
  • [P.F] 语言陷阱 — 禁止可变默认参数 → checklist-only
    auto_model.py:37 stop_id_list: list[int] = [] 使用可变 list 作为默认参数。方法体内有 .copy() 防御性拷贝,且为预先存在代码(本 PR 未引入),不升级为独立 issue。

Strengths

  • Host 侧 tensor 快照设计(_host_for_log 字段)巧妙避免了 dump 时对 GPU tensor 的同步开销,仅对少数无法预快照的 tensor 走 .cpu() 路径
  • 原子写入模式(tmp 文件写入 + rename)保证 dump 文件完整性,避免消费方读到半写文件
  • std::call_once + std::atomic 保证多线程安全,不引入锁竞争
  • ConfigInit.cc pickle 反序列化正确处理 t.size()==12 和 t.size()==13 两种情况,兼容旧版本
  • 测试覆盖完整:C++ DumpTool 验证序列化正确性,replay_test 验证 torch.load 可还原并支持模型 forward 回放

@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from 934e16a to 379615f Compare July 1, 2026 09:37
@LLLLKKKK

LLLLKKKK commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1118

Status: BLOCKING

Summary: P0/0 · P1/0 · P2/5 · P3/1

Non-blocking Suggestions

P2

  • CUDA tensor 被静默丢弃为 None,回放时无明确提示 @ rtp_llm/cpp/models/ModelInputsLogger.cc:57
    • 建议:建议对被跳过的 CUDA tensor 至少在首次遇到时输出一条 WARNING 日志(可用计数器限频),提示用户哪些字段因在 GPU 上而未被 dump。或者考虑对 kv_cache_block_id 等关键字段也在 Gatherer 中捕获 host 快照。
  • backup_count_ <= 0 时 rotated 文件永远不会被清理 @ rtp_llm/cpp/models/ModelInputsLogger.cc:186
    • 建议:当 backup_count_ == 0 时,cleanupOldFiles 应删除所有 rotated 文件,而不是跳过清理。可以将 if (backup_count_ <= 0) return; 改为仅当 backup_count_ < 0 时 return(表示无限保留),或将 backup_count_ == 0 解释为"只保留当前文件"。
  • torch.load(weights_only=False) 存在安全隐患 @ rtp_llm/cpp/models/test/model_inputs_logger_replay_test.py:40
    • 建议:在文档或日志中明确提示用户仅加载可信来源的 dump 文件。如果可行,考虑使用 weights_only=True 并注册必要的安全类,或添加文件来源校验。
  • write() 未检查 output_.write() 的写入结果 @ rtp_llm/cpp/models/ModelInputsLogger.cc:138
    • 建议:在 output_.write() 之后检查 output_.fail(),如果写入失败则记录 WARNING 并将 valid_ 设为 false 以停止后续写入,避免产生损坏的 dump 文件。
  • _init_dsv4_kv_cache 中硬编码了 DSv4 特定的 stride 计算公式 @ rtp_llm/models_py/standalone/auto_model.py:210
    • 建议:考虑将这些 stride 计算公式提取为共享的配置或工具函数,或者在 dump 文件中直接记录 stride 信息以便回放时使用,减少跨语言重复。

P3

  • stop_id_list 使用可变默认参数 @ rtp_llm/models_py/standalone/auto_model.py:37
    • 建议:将默认值改为 None,在函数体中处理:stop_id_list = stop_id_list or []

Checklist Violations (6 fail / 56 total)

General Principles Checklist

  • [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue CUDA tensor 被静默丢弃为 None,回放时无明确提示
    addTensor 中当 source.is_cuda() 为 true 时,直接插入空 c10::IValue(),导致 lm_output_indexeskv_cache_block_id 等未提供 host 快照的 tensor 在 dump 文件中全部为 None。回放端 _is_replayable_payload 检查 kv_cache_block_id 是否为 tensor,如果被静默丢弃则整条 record 不可回放。用户在线上开启 dump 后如果这些 tensor 已在 CUDA 上,会得到一份大量字段为空的 dump 文件,且无任何日志提示哪些字段被跳过。
  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue ``backup_count_ <= 0 时 rotated 文件永远不会被清理
    `cleanupOldFiles()` 在 `backup_count <= 0` 时直接 return,不执行任何清理。而 `backup_count_` 来自 `log_file_backup_count`(默认值 16),但构造函数用 `std::max(backup_count, 0)` 处理后,当用户显式传入 0 或负值时不会清理任何旧文件。每次轮转都生成新的 timestamp 文件,长时间运行可能填满磁盘。建议 `backup_count <= 0` 的语义应为"不保留备份"(清理所有旧文件),而非"不做清理"。_
  • [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue ``write()未检查output_.write()` 的写入结果`
    `Writer::write()` 调用 `output.write()` 后未检查流状态(`output_.good()` 或 `output_.fail()`),如果磁盘满、IO 错误等导致写入失败,`bytes_` 计数仍会增加,后续所有写入都不会触发轮转(因为 `bytes_` 虚高)。同时写入失败的数据会导致 dump 文件损坏(记录长度已写入但数据不完整)。_
  • [6.1] Software Engineering — DRY:重复非平凡逻辑被抽取或显式复用 → issue ``_init_dsv4_kv_cache 中硬编码了 DSv4 特定的 stride 计算公式
    _`init_dsv4_kv_cache` 方法中硬编码了大量 DSv4 特定的计算逻辑(如 `csa_kv_stride = align_dsv4_fp8_kv_bytes((kernel_tokens // 4) * 584)`、`state_ring` 等),这些常量和公式与 C++ 侧的 KV cache 初始化逻辑存在重复。如果 C++ 侧的 cache 布局发生变化,Python 侧需要同步修改,容易导致不一致。

Python Static-First Checklist

  • [P.E] 安全 — 禁止 pickle/eval/exec 处理不可信数据 → issue ``torch.load(weights_only=False) 存在安全隐患
    _`load_model_inputs_dump_records` 中使用 `torch.load(..., weights_only=False)` 加载 dump 文件。`weights_only=False` 允许 pickle 反序列化任意 Python 对象,如果 dump 文件被篡改或来自不可信来源,可能导致任意代码执行。虽然目前仅在测试中使用且 dump 文件由自身产生,但该工具设计为可加载外部 dump 文件(通过 `MODEL_INPUTS_DUMP_PATH` 环境变量),存在被恶意文件利用的风险。
  • [P.F] 语言陷阱 — 禁止可变默认参数 → issue ``stop_id_list 使用可变默认参数
    `AutoModel.init` 的参数 `stop_id_list: list[int] = []` 使用了可变默认参数。虽然当前代码中通过 `stop_id_list.copy()` 避免了直接修改默认值的问题,但这是一个已知的 Python 陷阱,PEP 和 linter 通常建议使用 `None` 作为默认值。

Strengths

  • 配置开关 enable_model_inputs_log 默认关闭,不影响线上性能,开启后可有效复现和调试推理问题。
  • Writer 内部类封装了文件轮转、大小限制、备份清理逻辑,职责单一且隔离良好。
  • 为 combo_tokens/input_lengths/sequence_lengths/prefix_lengths 预先在 Gatherer 中捕获 host 快照,避免 CUDA tensor 在 forward 中被覆写后无法 dump 的问题。
  • pickle unpickle 兼容性处理完善,新增字段后通过 tuple size 判断实现了向后兼容。
  • 提供了完整的 C++ dump tool + Python replay test,端到端覆盖了序列化/反序列化/回放链路。
  • AutoModel 扩展支持 DSv4 多 region KV cache 和 initialize() 初始化路径,提升了 standalone 调试能力。

@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from 379615f to 1678e56 Compare July 3, 2026 02:29
Copilot AI review requested due to automatic review settings July 3, 2026 02:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.

max_total_tokens: int = 4096,
tokens_per_block: int = 64,
kernel_tokens_per_block: int = 0,
stop_id_list: list[int] = [],
Comment on lines +209 to +213
ratios = list(self.model_config.attn_config.layer_compress_ratios)
kernel_tokens = self.kv_cache.kernel_seq_size_per_block
physical_tokens = self.kv_cache.seq_size_per_block
kernel_blocks_per_kv_block = max(1, physical_tokens // kernel_tokens)
head_dim = self.model_config.attn_config.size_per_head
Comment on lines +344 to +350
attention_inputs.kv_cache_kernel_block_id = (
attention_inputs.kv_cache_block_id
)
attention_inputs.kv_cache_kernel_block_id = attention_inputs.kv_cache_block_id
Comment on lines 376 to 388
attention_inputs.kv_cache_block_id_device = torch.tensor(
[[i for i in range(1, need_block_nums + 1)]],
dtype=torch.int32,
device=self.device,
)
attention_inputs.kv_cache_kernel_block_id_device = (
attention_inputs.kv_cache_block_id_device
)
attention_inputs.kv_cache_block_id = torch.tensor(
[[i for i in range(1, need_block_nums + 1)]], dtype=torch.int32
)
attention_inputs.kv_cache_kernel_block_id = (
attention_inputs.kv_cache_block_id
)
attention_inputs.kv_cache_kernel_block_id = attention_inputs.kv_cache_block_id
attention_inputs.dtype = get_typemeta(self.kv_cache.kv_cache_base_by_layer[0])
Comment on lines +49 to +52
rtp_llm::ModelInputsLogger logger(/*rank_id=*/2, /*backup_count=*/1, nullptr);
for (int i = 0; i < dump_count; ++i) {
logger.log(rtp_llm::makeInputs());
}
@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from 1678e56 to 1818574 Compare July 3, 2026 03:11
@LLLLKKKK

LLLLKKKK commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1118

Status: BLOCKING

Summary: P0/0 · P1/0 · P2/5 · P3/3

Non-blocking Suggestions

P2

  • Writer 构造期 file_size 错误处理会导致启动即刻触发轮转 @ rtp_llm/cpp/models/ModelInputsLogger.cc:267
    • 建议:file_size 后显式检查 ec,失败时把 bytes_ 兜底为 0 并 RTP_LLM_LOG_WARNING 记录(或改用 try { file_size(file_path_) } catch (...) { bytes_ = 0; }),避免把错误值当作真实大小参与轮转决策。
  • MtpExecutor 中 target / draft / sp_prefill_draft 三模型共享同一个 logger 导致条目无法区分 @ rtp_llm/cpp/normal_engine/speculative/MtpExecutor.cc:200
    • 建议:要么在 logger 侧接受一个 role_tag"target" / "draft" / "sp_prefill_draft")写入 payload 或落到不同 prefix 的文件;要么直接为三个模型各起一个 ModelInputsLogger,backup 保留数在同 rank 下分摊。若坚持共享,也建议在 payload 里追加一个稳定的 model_role 字段供 replay 端过滤。
  • *_host_for_log 是引用别名而非快照,命名与语义不符 @ rtp_llm/cpp/normal_engine/NormalModelInputGatherer.cc:412
    • 建议:二选一:(a) 直接 .clone() 出一份真正的独立 host tensor(dump 属调试路径,clone 成本可接受,也天然消除“将来某天被原地改”的隐患);(b) 保留引用语义,但把字段改名(例如 prefix_lengths_alias_for_log)并加一行注释注明“共享底层 storage,仅在 pinned buffer 从 gather 到 log 期间不被修改的前提下有效”。
  • AutoModel._init_dsv4_kv_cache 把 DSV4 特化常量硬编码进通用类 @ rtp_llm/models_py/standalone/auto_model.py:198
    • 建议:把 DSV4 特化逻辑抽到 rtp_llm/models_py/standalone/ 下的独立模块(例如 AutoModelDsv4KVCacheInitializer),AutoModel 通过某种注册/选择器分派(模型类型 → 初始化器)。至少加注释说明 576align_dsv4_fp8_kv_bytes 的对齐单位、584 的字节含义、state_ring 的 overlap/gen_num 语义,并统一 dict 键类型(推荐直接用枚举做键)。
  • replay 中 fp8_kv_cache 用环境变量默认开启,未从 dump 推导 @ rtp_llm/cpp/models/test/model_inputs_logger_replay_test.py:315
    • 建议:replay 侧从 payload 推导 fp8_kv_cache(比如根据 kv_block_stride_bytes 与 head 参数是否一致来判定),或者在 dump payload 里显式记一份 kv_cache_config 序列化片段(kv_cache_dtypeuse_mlalayer_compress_ratios 等),replay 读到不同精度时至少 warnings.warnraise

P3

  • ProfilingDebugLoggingConfig pickle 兼容矩阵存在冗余的 13-tuple 分支 @ rtp_llm/cpp/pybind/ConfigInit.cc:559
    • 建议:去掉 13-tuple 分支,只保留 12/15/16 三个已在历史里落地过的元组尺寸;或者把整个 unpickle 改成基于版本号 tag 的显式协议(例如 py::make_tuple("v2", ...)),避免继续膨胀 tuple 长度矩阵。
  • 帧化解析异常被吞并降级到整文件 torch.load,可能掩盖损坏 @ rtp_llm/cpp/models/test/model_inputs_logger_replay_test.py:47
    • 建议:要么把默认 .pt 一并纳入“必须走帧解析”的强规则并在异常时 raise 附加“帧头/长度信息”上下文;要么在 fallback 前先探测文件前 8 字节是否像帧长(< 100 * 1024 * 1024),只有明显不是帧格式才回退到单 torch.load。
  • enable_model_inputs_log 缺少显式端到端测试 @ rtp_llm/cpp/models/test/model_inputs_logger_replay_test.py:414
    • 建议:补一个最小 py_test:起 AutoModel 或 mock PyWrappedModel,模拟 enable_model_inputs_log=True 走完 gatherer → logger → 磁盘,assert 文件生成 + 内容能被 _load_model_inputs_dump_records 解析。或者在既有 replay test 里增加一段“不依赖外部路径”的最小 dump-生成分支。

Checklist Violations (12 fail / 56 total)

General Principles Checklist

  • [6.1] Architecture — 兼容性:公开 API/持久数据/配置/环境迁移安全 → issue ProfilingDebugLoggingConfig pickle 兼容矩阵存在冗余的 13-tuple 分支
    反序列化条件放宽为 t.size() != 12 && t.size() != 13 && t.size() != 15 && t.size() != 16。真实历史上应只存在两种旧格式(12=旧无 timeline 字段,15=旧无 model_inputs 字段)与一种新格式(16)。t.size() == 13 这条分支意味着“有 model_inputs 但没有 timeline 字段”,本仓库不会主动 dump 这种组合(py::make_tuple 一定是 16 元),除非有人手工构造。这条兼容路径不但没实际来源,还让 if (t.size() == 12 || t.size() == 13)if (t.size() == 13) { c.enable_model_inputs_log = t[12].cast<bool>(); } 嵌套判定更难阅读,未来再加字段极易再叠一层。
  • [6.1] Architecture — 分层边界:新概念在正确层级,不泄漏内部 → issue ``AutoModel._init_dsv4_kv_cache 把 DSV4 特化常量硬编码进通用类
    _新增的 `_init_dsv4_kv_cache` 在 `AutoModel` 里硬编码了 DeepSeekV4 相关的 magic number(`576`, `584`, `4`, `128`, region 顺序枚举),并通过 `if getattr(self.model_config.attn_config, "layer_compress_ratios", None): self.init_dsv4_kv_cache()` 靠鸭子类型分派。`AutoModel` 定位是通用 replay 入口,这样会让每新增一种特殊 KV layout 都要在同一个方法里加分支,违反 OCP/SRP,也让 `region_specs` 的常量来源(为什么是 576 对齐?为什么 CSA/HCA 用不同 stride?)完全无文档,未来维护成本高。此外 `region_specs` 用 `int(KVCacheRegionName.*)` 作 dict 键,与 `region_order` 用枚举本身两种索引方式并存,可读性差。
  • [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue ``*_host_for_log 是引用别名而非快照,命名与语义不符
    `if (config.enable_model_inputs_log) { model_input.prefix_lengths_host_for_log = model_input.prefix_lengths; }`(同处 `combo_tokens_host_for_log` 等)仅做 `torch::Tensor` 拷贝构造,两者共享同一底层 storage。当前调用链虽然不会 in-place 覆盖这些 host 张量,但字段名 `host_for_log` 强烈暗示“已快照的 host 副本”,未来任何在 gather→forward→log 之间对 pinned buffer 的原地改动(例如为节省分配复用 buffer)都会静默污染 dump。缺注释说明“持有原引用即可,因为下游不会 in-place 修改”,也没有 `TORCH_CHECK` 断言。
  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue ``*_host_for_log 是引用别名而非快照,命名与语义不符
    `if (config.enable_model_inputs_log) { model_input.prefix_lengths_host_for_log = model_input.prefix_lengths; }`(同处 `combo_tokens_host_for_log` 等)仅做 `torch::Tensor` 拷贝构造,两者共享同一底层 storage。当前调用链虽然不会 in-place 覆盖这些 host 张量,但字段名 `host_for_log` 强烈暗示“已快照的 host 副本”,未来任何在 gather→forward→log 之间对 pinned buffer 的原地改动(例如为节省分配复用 buffer)都会静默污染 dump。缺注释说明“持有原引用即可,因为下游不会 in-place 修改”,也没有 `TORCH_CHECK` 断言。
  • [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue 帧化解析异常被吞并降级到整文件 torch.load,可能掩盖损坏
    _load_model_inputs_dump_records 内部 try: load_framed_records() ... except Exception: if path.suffix == ".ptlog": raise; return [torch.load(path, ...)]。当扩展名是 .pt(正是 ModelInputsLogger 写出来的默认后缀)时,任何帧解析异常(比如帧长错乱意味着文件损坏)都会被静默 fallback 成整文件 torch.load,这在大概率下会再次抛出,但错误信息不再指向“帧解析失败”,而是 pickle 头错误,排查方向被误导;即使 torch.load 侥幸成功,也可能把损坏的多帧数据当成一个奇怪的 payload 返回。
  • [6.1] Quality — 逻辑变更未混入无关格式化 → issue ProfilingDebugLoggingConfig pickle 兼容矩阵存在冗余的 13-tuple 分支
    反序列化条件放宽为 t.size() != 12 && t.size() != 13 && t.size() != 15 && t.size() != 16。真实历史上应只存在两种旧格式(12=旧无 timeline 字段,15=旧无 model_inputs 字段)与一种新格式(16)。t.size() == 13 这条分支意味着“有 model_inputs 但没有 timeline 字段”,本仓库不会主动 dump 这种组合(py::make_tuple 一定是 16 元),除非有人手工构造。这条兼容路径不但没实际来源,还让 if (t.size() == 12 || t.size() == 13)if (t.size() == 13) { c.enable_model_inputs_log = t[12].cast<bool>(); } 嵌套判定更难阅读,未来再加字段极易再叠一层。
  • [6.1] Software Engineering — OCP:本地扩展点优先于修改中心逻辑 → issue ``AutoModel._init_dsv4_kv_cache 把 DSV4 特化常量硬编码进通用类
    _新增的 `_init_dsv4_kv_cache` 在 `AutoModel` 里硬编码了 DeepSeekV4 相关的 magic number(`576`, `584`, `4`, `128`, region 顺序枚举),并通过 `if getattr(self.model_config.attn_config, "layer_compress_ratios", None): self.init_dsv4_kv_cache()` 靠鸭子类型分派。`AutoModel` 定位是通用 replay 入口,这样会让每新增一种特殊 KV layout 都要在同一个方法里加分支,违反 OCP/SRP,也让 `region_specs` 的常量来源(为什么是 576 对齐?为什么 CSA/HCA 用不同 stride?)完全无文档,未来维护成本高。此外 `region_specs` 用 `int(KVCacheRegionName.*)` 作 dict 键,与 `region_order` 用枚举本身两种索引方式并存,可读性差。
  • [6.1] Software Engineering — SRP:模块/类职责单一 → issue ``AutoModel._init_dsv4_kv_cache 把 DSV4 特化常量硬编码进通用类
    _新增的 `_init_dsv4_kv_cache` 在 `AutoModel` 里硬编码了 DeepSeekV4 相关的 magic number(`576`, `584`, `4`, `128`, region 顺序枚举),并通过 `if getattr(self.model_config.attn_config, "layer_compress_ratios", None): self.init_dsv4_kv_cache()` 靠鸭子类型分派。`AutoModel` 定位是通用 replay 入口,这样会让每新增一种特殊 KV layout 都要在同一个方法里加分支,违反 OCP/SRP,也让 `region_specs` 的常量来源(为什么是 576 对齐?为什么 CSA/HCA 用不同 stride?)完全无文档,未来维护成本高。此外 `region_specs` 用 `int(KVCacheRegionName.*)` 作 dict 键,与 `region_order` 用枚举本身两种索引方式并存,可读性差。
  • [6.1] Tests — 新逻辑有聚焦单测 + 相关集成/smoke 测试 → issue ``enable_model_inputs_log 缺少显式端到端测试
    新增的 `model_inputs_logger_dump_tool` 只直接构造 `GptModelInputs` 调用 `ModelInputsLogger::log`,不涉及 `enable_model_inputs_log` 配置传递路径(server_args → ProfilingDebugLoggingConfig → NormalBatchStreamProcessor → NormalModelInputGatherer → PyWrappedModel)。`test_optional_external_dump_forward_replay` 依赖外部 dump 路径与本地模型文件,CI 无此环境时直接 skip。这样 `enable_model_inputs_log` 从命令行 → C++ 全链路的“打开开关就有 dump 文件”这个用户可感知契约没有回归保护,未来任何一环重构(例如 config 拆分、gatherer 抽象)都可能悄悄失效。
  • [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue ``enable_model_inputs_log 缺少显式端到端测试
    新增的 `model_inputs_logger_dump_tool` 只直接构造 `GptModelInputs` 调用 `ModelInputsLogger::log`,不涉及 `enable_model_inputs_log` 配置传递路径(server_args → ProfilingDebugLoggingConfig → NormalBatchStreamProcessor → NormalModelInputGatherer → PyWrappedModel)。`test_optional_external_dump_forward_replay` 依赖外部 dump 路径与本地模型文件,CI 无此环境时直接 skip。这样 `enable_model_inputs_log` 从命令行 → C++ 全链路的“打开开关就有 dump 文件”这个用户可感知契约没有回归保护,未来任何一环重构(例如 config 拆分、gatherer 抽象)都可能悄悄失效。

Python Static-First Checklist

  • [P.A] 静态结构与类型纪律 — 数据容器用 dataclass/NamedTuple/TypedDict → issue ``AutoModel._init_dsv4_kv_cache 把 DSV4 特化常量硬编码进通用类
    _新增的 `_init_dsv4_kv_cache` 在 `AutoModel` 里硬编码了 DeepSeekV4 相关的 magic number(`576`, `584`, `4`, `128`, region 顺序枚举),并通过 `if getattr(self.model_config.attn_config, "layer_compress_ratios", None): self.init_dsv4_kv_cache()` 靠鸭子类型分派。`AutoModel` 定位是通用 replay 入口,这样会让每新增一种特殊 KV layout 都要在同一个方法里加分支,违反 OCP/SRP,也让 `region_specs` 的常量来源(为什么是 576 对齐?为什么 CSA/HCA 用不同 stride?)完全无文档,未来维护成本高。此外 `region_specs` 用 `int(KVCacheRegionName.*)` 作 dict 键,与 `region_order` 用枚举本身两种索引方式并存,可读性差。
  • [P.B] 错误处理 — 禁止 bare except 或静默吞异常 → issue 帧化解析异常被吞并降级到整文件 torch.load,可能掩盖损坏
    _load_model_inputs_dump_records 内部 try: load_framed_records() ... except Exception: if path.suffix == ".ptlog": raise; return [torch.load(path, ...)]。当扩展名是 .pt(正是 ModelInputsLogger 写出来的默认后缀)时,任何帧解析异常(比如帧长错乱意味着文件损坏)都会被静默 fallback 成整文件 torch.load,这在大概率下会再次抛出,但错误信息不再指向“帧解析失败”,而是 pickle 头错误,排查方向被误导;即使 torch.load 侥幸成功,也可能把损坏的多帧数据当成一个奇怪的 payload 返回。

Strengths

  • ModelInputsLogger 采用 PIMPL 隐藏 Writer 细节,序列化/写入分别用 RTP_LLM_PROFILE_SCOPE 打点,便于线上排查。
  • Writer 用 std::mutex + record-length 前缀帧格式,回放侧对应 _load_model_inputs_dump_records 能正确按帧解析并向后兼容单 record torch.load 形式。
  • enable_model_inputs_log 默认关闭,热路径在 flag 未开启时零开销(只多一次指针空判);Failure path 用 try/catch 兜住 pickle/report 异常,不影响 forward 主流程。
  • Backup 清理按 prefix_ + "_" + timestamp 前缀严格过滤,避免误删同目录其它日志;文件名时间戳按 YYYYMMDD_HHMMSS_UUUUUU 便于字典序=时间序排序。
  • 新增 AutoModel.from_pretrained(..., init_tokenizer_and_lm_head=False) + _initialize_py_model 复用 model.initialize,让离线 replay 无需完整加载分词器/lm_head,减少 replay 依赖。

Copilot AI review requested due to automatic review settings July 3, 2026 07:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.

Comment on lines 15 to 19
from rtp_llm.ops.compute_ops import (
CacheGroupType,
KVCache,
KVCacheRegionName,
PyAttentionInputs,
Comment on lines 181 to 184
# Explicitly mark every layer as full-attention.
self.kv_cache.layer_attn_types = [
self.kv_cache.layer_group_types = [
CacheGroupType.FULL for _ in range(self.layer_num)
]
Comment on lines +260 to +264
self.kv_cache.group_region_names = region_order
self.kv_cache.group_seq_size_per_block = [physical_tokens for _ in region_order]
self.kv_cache.layer_region_to_group_id = [
[-1 for _ in range(region_count)] for _ in range(self.layer_num)
]
Comment on lines +265 to +267
self.kv_cache.layer_group_types = [
CacheGroupType.SWA for _ in range(self.layer_num)
]
@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from e42739f to 07ce2ed Compare July 3, 2026 09:01
Copilot AI review requested due to automatic review settings July 3, 2026 09:13
@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from 07ce2ed to 4ac7dcb Compare July 3, 2026 09:13
@JackTan25 JackTan25 force-pushed the feat/model_inputs_dump branch from 4ac7dcb to c7fef42 Compare July 3, 2026 09:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Comment on lines +8 to +54
namespace rtp_llm {
namespace {

GptModelInputs makeInputs() {
GptModelInputs inputs;
inputs.trace_ids = {"trace-a", "trace-b"};
inputs.combo_tokens = torch::tensor({11, 12, 13}, torch::kInt32);
inputs.input_lengths = torch::tensor({3}, torch::kInt32);
inputs.sequence_lengths = torch::tensor({2}, torch::kInt32);
inputs.lm_output_indexes = torch::tensor({2}, torch::kInt32);
inputs.prefix_lengths = torch::tensor({1}, torch::kInt32);
inputs.combo_tokens_type_ids = torch::tensor({0, 0, 1}, torch::kInt32);
inputs.combo_position_ids = torch::tensor({0, 1, 2}, torch::kInt32);
inputs.kv_cache_block_id = torch::tensor({{{7, 8}}}, torch::kInt32);
inputs.kv_cache_layer_to_group = torch::tensor({0}, torch::kInt32);
inputs.kv_cache_group_types = torch::tensor({1}, torch::kInt32);
inputs.kv_cache_update_mapping = torch::tensor({{1, 2}}, torch::kInt32);
inputs.request_id = torch::tensor({12345}, torch::kInt64);
inputs.request_pd_separation = torch::tensor({false}, torch::kBool);
inputs.kv_block_stride_bytes = 4096;
inputs.kv_scale_stride_bytes = 128;
inputs.seq_size_per_block = 64;
inputs.kernel_seq_size_per_block = 32;
inputs.decode_entrance = true;
inputs.need_all_logits = true;
inputs.need_moe_gating = true;
return inputs;
}

} // namespace
} // namespace rtp_llm

int main(int argc, char** argv) {
if (argc != 2 && argc != 3) {
std::cerr << "usage: " << argv[0] << " <log_dir> [dump_count]" << std::endl;
return 2;
}

const auto dump_count = argc == 3 ? std::stoi(argv[2]) : 1;
setenv("LOG_PATH", argv[1], 1);
setenv("FRONTEND_SERVER_ID", "3", 1);
rtp_llm::ModelInputsLogger logger(/*rank_id=*/2, /*backup_count=*/1, nullptr);
for (int i = 0; i < dump_count; ++i) {
logger.log(rtp_llm::makeInputs());
}
return 0;
}
Comment on lines +163 to +183
void rotateIfNeeded(size_t next_bytes) {
if (bytes_ == 0 || bytes_ + next_bytes <= kModelInputsDumpMaxBytes) {
return;
}

output_.close();
const auto rotated_path =
output_dir_ / (prefix_ + "_" + timestampForFile(autil::TimeUtility::currentTimeInMicroSeconds()) + ".pt");
std::error_code ec;
std::filesystem::rename(file_path_, rotated_path, ec);
if (ec) {
RTP_LLM_LOG_WARNING("Failed to rotate model inputs dump file from %s to %s: %s",
file_path_.c_str(),
rotated_path.c_str(),
ec.message().c_str());
}
cleanupOldFiles();
bytes_ = 0;
pending_records_ = 0;
open(std::ios::out | std::ios::binary | std::ios::trunc);
}
Comment on lines +185 to +211
void cleanupOldFiles() {
if (backup_count_ <= 0) {
return;
}

std::vector<std::filesystem::directory_entry> rotated_files;
std::error_code ec;
for (const auto& entry : std::filesystem::directory_iterator(output_dir_, ec)) {
if (ec || !entry.is_regular_file()) {
continue;
}
const auto name = entry.path().filename().string();
if (name.rfind(prefix_ + "_", 0) == 0 && entry.path().extension() == ".pt") {
rotated_files.push_back(entry);
}
}
if (rotated_files.size() <= static_cast<size_t>(backup_count_)) {
return;
}
std::sort(rotated_files.begin(), rotated_files.end(), [](const auto& lhs, const auto& rhs) {
return lhs.path().filename().string() < rhs.path().filename().string();
});
const auto remove_count = rotated_files.size() - static_cast<size_t>(backup_count_);
for (size_t i = 0; i < remove_count; ++i) {
std::filesystem::remove(rotated_files[i], ec);
}
}
Comment on lines +412 to +414
if (config_.enable_model_inputs_log) {
model_input.prefix_lengths_host_for_log = model_input.prefix_lengths;
}
Copilot AI review requested due to automatic review settings July 3, 2026 09:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@LLLLKKKK

LLLLKKKK commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1118

Status: BLOCKING

Summary: P0/0 · P1/0 · P2/3 · P3/2

Non-blocking Suggestions

P2

  • MTP decode 路径下 host_for_log 快照会与真实 forward 输入错位 @ rtp_llm/cpp/normal_engine/speculative/MtpExecutor.cc:595
    • 建议:在 MTP 的 prepareOneStepSpecDecodeModelInputprepareDecodeDraftModelInputupdateDecodeDraftModelInputupdateDecodePostDraftModelInput 等修改 combo_tokens/sequence_lengths/prefix_lengths/input_lengths 的位置,同步更新对应 _host_for_log 字段;或者在 ModelInputsLogger::log 内加一条一致性判断:当 host_snapshot 与当前 tensor 的 storage 不同时优先使用当前 tensor(若非 CUDA)。同时在 enable_model_inputs_log 打开时补一条 MTP decode 的 replay 断言,避免回归。
  • 日志文件 rotate 时 rename 失败仍会 trunc 现有文件导致数据丢失 @ rtp_llm/cpp/models/ModelInputsLogger.cc:304
    • 建议:在 rename 失败分支里 early-return(保留 file_path_ 内容,重新以 std::ios::app 打开、bytes_ 保持原值),或至少改用 std::ios::out | std::ios::binary | std::ios::app 兜底并跳过 cleanupOldFiles,同时把失败情况反映到 valid_ 或 metric 供上游告警。
  • dump payload 缺失 lm_output_lengths、多模态与 cache_keys 字段,多模态 replay 无法完整复现 @ rtp_llm/cpp/models/ModelInputsLogger.cc:68
    • 建议:将 lm_output_lengthstext_tokens_maskmm_features_locsmultimodal_features(每个 tensor 分别处理 host/CUDA 情况)、mm_extra_inputcache_keys 补入 payload;对 std::vector<torch::Tensor> 使用 c10::List 序列化。同时扩展 replay 测试覆盖一个多模态样例的落盘/回读断言,避免后续 GptModelInputs 新增字段被再次遗漏。

P3

  • host_for_log 快照的写入被拆到两个位置,语义不一致 @ rtp_llm/cpp/normal_engine/NormalModelInputGatherer.cc:412
    • 建议:把 4 处 host_for_log 快照统一到 gather() 结尾一处(或抽出 snapshotHostForLog(model_input) 私有方法),并把 enable_model_inputs_log 判断内联到那里;保留一处即可,语义与失效范围都更清晰。
  • enable path 会同步 pickle 全量 tensor,缺少大 payload 熔断 @ rtp_llm/cpp/models/ModelInputsLogger.cc:230
    • 建议:在 enable_model_inputs_log=true 的文档/help 中显式提示"仅供离线调试,会显著增加 forward 延迟";进一步可引入按 tensor 大小的截断(例如 attention_mask/kv_cache_block_id 超过阈值只落 shape+dtype),或把 write_file 放到独立线程做 SPSC 队列,避免持锁写盘阻塞 forward。

Checklist Violations (5 fail / 56 total)

General Principles Checklist

  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue MTP decode 路径下 host_for_log 快照会与真实 forward 输入错位
    NormalModelInputGatherer::gathercombo_tokens_host_for_log = model_input.combo_tokens 是浅拷贝,与当时的 pinned host tensor 共享 storage。而 MTP decode 路径在 gather 后会调用 prepareOneStepSpecDecodeModelInputprepareDecodeDraftModelInputMtpBatchStreamProcessor.cc:194、163),把 model_input.combo_tokens 整体替换成一个全新 tensor(sequence_lengthsprefix_lengths 亦被替换或置空)。随后 model_->forward(model_input)MtpExecutor.cc:627)与 draft forward 触发 ModelInputsLogger::logaddTensorhost_snapshot.defined()
  • [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue 日志文件 rotate 时 rename 失败仍会 trunc 现有文件导致数据丢失
    rotateIfNeeded 中先 close 当前文件,std::filesystem::rename(file_path_, rotated_path, ec) 失败时仅 RTP_LLM_LOG_WARNING,随后无论成功与否都执行 cleanupOldFilesbytes_=0open(std::ios::out | std::ios::binary | std::ios::trunc)。当 rename 失败(例如 rotated_path 已存在、跨 mount、无权限)时 file_path_ 仍是历史累积的完整数据文件,被立刻以 trunc 打开会清空原有 100MB 内容;同时 bytes_ 重置为 0,接下来其他记录继续写入,历史数据无法找回。
  • [6.1] Quality — 无 per-forward 调试日志 / 噪声热路径输出 → issue enable path 会同步 pickle 全量 tensor,缺少大 payload 熔断
    ModelInputsLogger::log 在 forward 关键路径同步执行 buildModelInputsPayload → pickle_save → write,pickle_save 会把 attention_maskkv_cache_block_id(多 group×batch×blocks)等潜在大 tensor 全量拷贝到内存缓冲后写盘,同时持有 Writer 的 mutex_。对于长上下文或多 rank 大 batch,这会明显拉长 P99 forward 延迟;目前只有 metric 观测,没有超时 / 大小硬阈值兜底。文件也没有 fsync/后台线程写入,kModelInputsDumpFlushIntervalUs=100ms 只影响 flush 频率。
  • [6.1] Software Engineering — DRY:重复非平凡逻辑被抽取或显式复用 → issue host_for_log 快照的写入被拆到两个位置,语义不一致
    prefix_lengths_host_for_logprocessContextStreams 尾部赋值(NormalModelInputGatherer.cc:412-413),而 combo_tokens_host_for_log/input_lengths_host_for_log/sequence_lengths_host_for_log 又在 gather() 尾部单独赋值(NormalModelInputGatherer.cc:427-436)。四个快照本质上做同一件事,却分散在两个函数里,未来任何新增字段都容易被漏掉一个入口;也让上一条 MTP staleness 问题的修复面更大。
  • [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue dump payload 缺失 lm_output_lengths、多模态与 cache_keys 字段,多模态 replay 无法完整复现
    buildModelInputsPayload 仅覆盖 28 个字段。GptModelInputs 中的 lm_output_lengths(forward 与 dispatch 均使用)、text_tokens_maskmm_features_locsmultimodal_featuresmm_extra_inputcache_keys 均未落盘。同时 replay 测试 expected_keys 也刚好等于这 28 项,仅能约束现有覆盖不缩水,无法暴露多模态请求或需要 lm_output_lengths 的场景无法复现的问题。这令 --enable_model_inputs_log 在多模态或 PD-sep 场景上失去主要用途。

Strengths

  • 特性通过 enable_model_inputs_log 显式开关控制,默认关闭,对生产路径无影响。
  • ModelInputsLogger::log 全流程包裹 try/catch,同时对 metrics report 单独 try/catch,避免调试特性影响 forward 主流程。
  • ProfilingDebugLoggingConfig 的 pickle 兼容旧的 12 元素与 15 元素 tuple,避免升级期反序列化失败。
  • 文件名 model_inputs_r<rank>_s<server> 天然区分多 rank 与多 frontend 场景,rotate 时用时间戳 + 微秒后缀,避免文件冲突。
  • CUDA tensor 检测(source.is_cuda())避免落盘无法在 CPU 侧反序列化的设备内存。
  • 添加 RTP_LLM_PROFILE_SCOPE 到 build_payload/pickle_save/write_file/report_metric 各阶段,便于观测每一步耗时。
  • 单测 model_inputs_logger_replay_test.py 覆盖了帧格式解析、字段完整性断言与 kernel block id 派生。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants